Skip to content

FE-502: Improve SDCPN file import/export with Zod validation and layout fix#8536

Open
kube wants to merge 4 commits intocf/petrinaut-copy-pastefrom
cf/petrinaut-better-export-import
Open

FE-502: Improve SDCPN file import/export with Zod validation and layout fix#8536
kube wants to merge 4 commits intocf/petrinaut-copy-pastefrom
cf/petrinaut-better-export-import

Conversation

@kube
Copy link
Collaborator

@kube kube commented Mar 12, 2026

🌟 What is the purpose of this PR?

Fixes the bug where importing an SDCPN file exported without visual info breaks the editor. Also improves the file format with Zod validation and a versioned envelope.

🔗 Related links

🔍 What does this change?

  • Moves file format code into src/file-format/ (export-sdcpn, import-sdcpn, remove-visual-info, old-formats/)
  • Replaces manual type guards with Zod schemas for import validation, matching the pattern used for clipboard in clipboard/types.ts
  • Adds versioned file format envelope (version, meta.generator) to exported files
  • Fixes layout on import: when nodes are missing x/y, runs ELK layout before calling createNewNet (avoids a stale closure bug where positions were applied to the wrong net)
  • Adds onlyMissingPositions option to calculateGraphLayout for partial re-layout of imported nodes
  • Adds ImportErrorDialog showing Zod validation errors with a "Create empty net" fallback

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

❓ How to test this?

  1. Export a net as "JSON without visual info"
  2. Import the exported file back
  3. Confirm all nodes are laid out by ELK (not stacked at origin)
  4. Import a malformed JSON file and confirm the error dialog appears

@vercel
Copy link

vercel bot commented Mar 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hash Ready Ready Preview, Comment Mar 16, 2026 9:47am
petrinaut Ready Ready Preview Mar 16, 2026 9:47am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hashdotdesign Ignored Ignored Preview Mar 16, 2026 9:47am
hashdotdesign-tokens Ignored Ignored Preview Mar 16, 2026 9:47am

@github-actions github-actions bot added area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team labels Mar 12, 2026
Copy link
Collaborator Author

kube commented Mar 12, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kube kube marked this pull request as ready for review March 12, 2026 23:15
@cursor
Copy link

cursor bot commented Mar 12, 2026

PR Summary

Medium Risk
Introduces a new versioned file envelope and stricter Zod-based import validation, which can affect compatibility with previously exported files and error handling paths. Also changes import-time layout behavior by conditionally running ELK, so regressions could impact editor loading and node positioning.

Overview
Adds a versioned SDCPN JSON file format for exports (wraps net data with version and meta.generator) and introduces Zod schemas (sdcpnFileSchema, legacySdcpnFileSchema, plus a pre-2025-11-28 schema) to validate and parse imports with explicit rejection of unsupported versions.

Fixes importing files exported without visual info by filling missing x/y and type display fields with defaults, then running ELK layout only for nodes missing positions via a new calculateGraphLayout(..., { onlyMissingPositions }) option before creating the new net.

Updates the editor import flow to be async, surfaces validation/read errors in a new ImportErrorDialog (with a “Create empty net” fallback), and removes the old alert-based import helper while rehoming/rewiring old-format conversion exports under file-format/.

Written by Cursor Bugbot for commit 28e2ea1. This will update automatically on new commits. Configure here.

@augmentcode
Copy link

augmentcode bot commented Mar 12, 2026

🤖 Augment PR Summary

Summary: This PR refactors Petrinaut’s SDCPN JSON import/export to be versioned and Zod-validated, and improves the import experience when node positions are missing.

Changes:

  • Moved SDCPN file I/O helpers into src/file-format/ (export, import, remove-visual-info, old-format converters).
  • Introduced a v1 on-disk JSON envelope with version and meta.generator metadata on export.
  • Added Zod schemas for both the versioned format and a legacy (unversioned) format, using defaults for optional sections.
  • Reworked import to return a structured result (success/error) and to detect missing x/y coordinates.
  • On import, fills missing coordinates and (when needed) runs ELK layout before calling createNewNet to avoid stale-closure issues.
  • Extended calculateGraphLayout with an onlyMissingPositions option to support partial re-layout.
  • Added an ImportErrorDialog to display validation/read errors with a “Create empty net” fallback.

Technical Notes: The new importer prefers the versioned schema first, falls back to the legacy schema, and uses user settings (compact vs classic node dimensions) when computing ELK layout.

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 3 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

const needsPosition = new Set<string>();
if (options?.onlyMissingPositions) {
for (const place of sdcpn.places) {
if (place.x === 0 && place.y === 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In onlyMissingPositions mode, a node is treated as “missing” only when both x and y are 0, but the import path can produce partially-missing coordinates (e.g. x filled to 0 while y was present), which would then never get a new position. This can leave imported nodes stuck at a 0 coordinate even though hadMissingPositions triggered layout.

Severity: medium

Other Locations
  • libs/@hashintel/petrinaut/src/file-format/import-sdcpn.ts:38
  • libs/@hashintel/petrinaut/src/views/Editor/editor-view.tsx:147

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

}

// Fall back to legacy format
const legacy = legacySdcpnFileSchema.safeParse(data);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseSDCPNFile currently requires the file to match the current SDCPN shape (colorId, differentialEquationId, etc.) before convertOldFormatToSDCPN runs, so pre-2025-11-28 exports (which use fields like place.type) will fail validation and never be migrated. If the goal is to keep old-file import working, this validation step may be too strict for legacy inputs.

Severity: medium

Other Locations
  • libs/@hashintel/petrinaut/src/file-format/types.ts:10

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

/**
* Schema for the legacy file format (no version/meta, just title + SDCPN data).
*/
export const legacySdcpnFileSchema = sdcpnSchema.extend({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

legacySdcpnFileSchema is non-strict, so a file that includes version/meta but fails sdcpnFileSchema (e.g., version > supported) can still be accepted as “legacy” after those keys are stripped. That could silently import future/incompatible versioned files instead of surfacing an error.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@kube kube changed the title Improve SDCPN file import/export with Zod validation and layout fix FE-502: Improve SDCPN file import/export with Zod validation and layout fix Mar 13, 2026
@kube kube changed the base branch from cf/petrinaut-copy-paste to graphite-base/8536 March 14, 2026 19:32
@kube kube force-pushed the cf/petrinaut-better-export-import branch from 82c0b1c to 5cb778f Compare March 14, 2026 19:48
@kube kube force-pushed the graphite-base/8536 branch from 7889b18 to 1a404fc Compare March 14, 2026 19:48
@kube kube changed the base branch from graphite-base/8536 to cf/petrinaut-copy-paste March 14, 2026 19:48
@kube kube marked this pull request as draft March 14, 2026 19:57
@kube kube marked this pull request as ready for review March 14, 2026 19:57
@kube kube force-pushed the cf/petrinaut-better-export-import branch from 5cb778f to 9d9196d Compare March 14, 2026 20:01
Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 4 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

* @returns A promise that resolves to an array of node positions
* @param dims - Node dimensions for places and transitions
* @param options.onlyMissingPositions - When true, only nodes with x=0 and y=0 will receive new positions.
* Nodes that already have non-zero positions are included in the layout graph (so ELK can route around them)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says existing non-zero nodes are included “so ELK can route around them,” but elkNodes never passes existing x/y (or any fixed-position hint) into the ELK graph. In onlyMissingPositions mode this can yield returned positions that don’t account for the unchanged nodes, potentially causing overlaps when only the “missing” nodes get applied.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

const needsPosition = new Set<string>();
if (options?.onlyMissingPositions) {
for (const place of sdcpn.places) {
if (place.x === 0 && place.y === 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In onlyMissingPositions mode, treating (0,0) as “missing” can also catch nodes that were intentionally positioned at the origin (if some other node had missing coords and triggered relayout). That could unexpectedly move a legitimately-placed node.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

useSelectionCleanup();

function handleNew() {
const handleCreateEmpty = useCallback(() => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useCallback is used for handleCreateEmpty, but Petrinaut’s React Compiler guidance (CLAUDE.md) asks to avoid manual memoization unless there’s a specific incompatibility. If there isn’t a concrete need for stable identity here, this is extra complexity and may be worth reconsidering.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

/**
* Parses raw JSON data into an SDCPN, handling both versioned and legacy formats.
*/
export const parseSDCPNFile = (data: unknown): ImportResult => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parseSDCPNFile is a key new parsing/validation surface, but I don’t see unit coverage for versioned vs legacy inputs or malformed JSON cases. Adding a small set of tests here would help lock in backward-compat and error-reporting behavior.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

kube and others added 4 commits March 16, 2026 10:32
- Move file format code into `src/file-format/` (export, import, remove-visual-info, old-formats)
- Replace manual type guards with Zod schemas for import validation, matching clipboard types pattern
- Add versioned file format envelope (version, meta.generator) to exports
- Fix stale closure bug: run ELK layout before createNewNet so positions are baked into the data
- Add `onlyMissingPositions` option to calculateGraphLayout for partial re-layout
- Add ImportErrorDialog showing Zod validation errors with "Create empty net" fallback

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Zod schema required iconSlug and displayColor on color types, but
these fields are stripped by "export without visual info". Make them
optional in the schema and fill defaults on import, matching how
missing x/y positions are already handled.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move old format Zod schema into old-formats/pre-2025-11-28/
- Integrate old format parsing into parseSDCPNFile so pre-2025-11-28
  files are validated and converted during import (previously dead code)
- Reject versioned files from legacy fallback path to prevent silently
  accepting unsupported future versions
- Remove dead convertOldFormatToSDCPN call from editor-view
- Remove unnecessary useCallback (React Compiler handles memoization)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the unsupported-version rejection from a Zod .refine() (which
never worked because z.object() strips unknown keys first) into
parseSDCPNFile where we can inspect the raw data. Remove unused
SDCPNFileFormat type export.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

}
}
return false;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing-position detection conflates types and positions

Medium Severity

hasMissingVisualInfo returns true when only type visual info (iconSlug/displayColor) is missing, even if all node positions are present. This causes handleImport to call calculateGraphLayout with onlyMissingPositions: true, which treats any node at (0, 0) as needing a new position. A node legitimately placed at the origin would be incorrectly re-positioned by ELK whenever unrelated type visual info is missing. The hadMissingVisualInfo flag conflates two distinct concerns — missing positions and missing type styling — but only the position concern is acted upon downstream.

Additional Locations (2)
Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team

Development

Successfully merging this pull request may close these issues.

1 participant